Repository navigation
Conversation
|
👋 感谢您对 RT-Thread 的贡献!Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范,请在你的仓库中执行以下步骤运行代码格式化工作流(如果格式化CI运行失败)。 🛠 操作步骤 | Steps
完成后,提交将自动更新至 如有问题欢迎联系我们,再次感谢您的贡献!💐 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
🏷️ Tag: kernelReviewers: @GorrayLi @ReviewSun @hamburger-os @lianux-mm @wdfk-prog @xu18838022837 Changed Files (Click to expand)
🏷️ Tag: workflowReviewers: @Rbb666 @kurisaW @supperthomas Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-11-14 21:32 CST)
📝 Review Instructions
|
|
BernardXiong
left a comment
There was a problem hiding this comment.
Review / 审查
结论 / Conclusion: no blocking issue found in the duplicate-defunct protection.
The new flag is initialized with the scheduler context, and _thread_mark_defunct() makes the check-and-set atomic under the thread spinlock before mutex detachment and defunct enqueue. This addresses the duplicate enqueue/double cleanup scenario described by the PR, and the change is small and localized.
建议 / Suggestions
-
Please add an SMP-focused regression case, or document why the existing lock ordering is sufficient. The paths now combine the scheduler critical section, the thread spinlock, and mutex detachment; a test should exercise concurrent
rt_thread_delete()/rt_thread_detach()against natural thread exit. -
The new bit lives in
rt_sched_thread_ctx, whose other fields are generally accessed under scheduler-lock rules, while_thread_mark_defunct()uses the thread spinlock. Please add a short comment documenting thatsched_flag_defunctis intentionally protected bythread->spinlock, so future scheduler changes do not introduce a lock-domain mismatch. -
Keep the negative return behavior covered for both dynamic and static threads after natural exit, including repeated calls. The current tests cover the main cases; the SMP/race case is the remaining useful gap.
The PR description and test intent are clear. / PR 描述和测试意图清晰。
|
@BernardXiong 熊大,已经按照修改建议修复了 |
745bcd9 to
ec6c0ec
Compare
| #endif /* RT_USING_MEM_PROTECTION */ | ||
|
|
||
| struct rt_spinlock spinlock; | ||
| rt_uint8_t defunct; /**< cleanup claimed, protected by spinlock */ |
There was a problem hiding this comment.
这里是否用一个原子操作类型,是否也可以?这样可以做无锁化处理
|
@BernardXiong 已更新,采用无锁方式实现,请继续审核 |
Replace the thread spinlock check-and-set with rt_atomic_exchange and initialize the claim with rt_atomic_store. Keep the existing cleanup ordering and repeated delete/detach error behavior. Sync with master and preserve loop-scaled monitor timeouts, assertion failure detection and process-substitution log reading. Validation: workflow YAML parsing, bash -n for all seven shell steps, and 11 log-monitor scenarios.
BernardXiong
left a comment
There was a problem hiding this comment.
结论:需要修改(仅格式问题);逻辑方向正确。 / Verdict: needs a format fix; the logic looks correct.
-
[阻塞] CI 格式检查失败:
Scan code format and license失败。请对新增/修改的src/utest/thread_tc.c、src/utest/thread_suspend_tc.c、components/drivers/ipc/utest/workqueue_tc.c按仓库tools下的 clang-format 配置格式化后重推。others_ft32_mm32_acm32的失败看起来停在工具链/依赖阶段,但格式化必须先处理。 -
[确认无误] defunct 认领:
_thread_mark_defunct()用rt_atomic_exchange()只允许一个清理者执行_thread_detach_from_mutex()和rt_thread_defunct_enqueue();_thread_init()在新生命周期开始时清零,静态/动态线程都会经过这里,逻辑闭合。
_thread_detach()中rt_thread_close()目前恒返回RT_EOK,因此把入队放进if (error == RT_EOK)不改变既有行为。 -
[建议] 用例覆盖:utest 已覆盖双路径竞争,请确认在
others_ft32_mm32_acm32失败排除后,带RT_USING_SMP的 QEMU 任务能实际跑过该用例,并在 PR 描述里贴一次运行结果。
English: the atomic cleanup claim is sound and _thread_init() resets it per lifetime. The only blocking item is the failing format check; please clang-format the new utest files. The others_ft32_mm32_acm32 failure looks environmental, but confirm the new SMP test actually runs and paste the result.
|
补充一个当前 CI 的阻塞项: 同一个 job 在 master 失败 job:https://github.com/RT-Thread/rt-thread/actions/runs/43348269374/job/131646192628 建议在合并前处理这个 BSP 的体积回归:可以压缩这条 cleanup 路径的代码体积,或与该 BSP 维护者确认是否调整链接脚本/默认配置的 ROM 预算。格式检查失败仍在,按上一条 review 处理即可。 |
BernardXiong
left a comment
There was a problem hiding this comment.
结论:格式问题已修复;新出现的 A9-smp 超时看起来与本轮增量无关,但 CI 未通过,故本轮不做完整审查。 / Verdict: format fix confirmed; the new A9-smp timeout appears unrelated to this delta, but CI is red so no full review this round.
head 49cfd2f1 相对上一轮审查的 8406af15 仅剩 hk32 调试优化 -O0 → -Og 与 rtdef.h 空白对齐,clang-format 问题已解决。
新失败 / New failure: A9-smp :kernel/ipc.cfg 在 utest 运行 20 分钟后超时,日志停在 core.mutex 的 test_dynamic_pri_reverse;本 PR 的 workqueue_tc 在同一 run 中已通过,且上一轮 head 8406af15 的同一 job 也通过。考虑到增量只有 -Og 与空白,暂判断为间歇性超时而非本 PR 逻辑引入:
建议重跑该配置;若复现,请作者结合 src/thread.c 的 defunct 清理路径定位。本轮不把该 SHA 记为已完整审查。
拉取/合并请求描述:(PR description)
为什么提交这份 PR (why to submit this PR)
线程自然退出、显式删除或分离后,在对象仍有效但尚未完成回收时再次调用
rt_thread_delete()/rt_thread_detach(),可能将同一线程重复加入 defunct 队列,造成链表破坏或重复清理。本 PR 为每个线程生命周期增加一次性清理保护。Prevent duplicate defunct enqueue and cleanup while the thread object remains valid.
你的解决方案是什么 (what is your solution)
struct rt_thread中增加独立的rt_atomic_t defunct,由_thread_init()使用rt_atomic_store()清零。_thread_mark_defunct()使用rt_atomic_exchange()原子取得清理权,只有新生命周期的初始化才清零。-RT_ERROR。Use an independent atomic cleanup claim, initialized with
rt_atomic_store()and acquired withrt_atomic_exchange(), so cleanup and enqueue run once per lifetime. Freed handles and remote reclamation of running threads are outside this change.修改范围 / Changed files
include/rtdef.h、src/thread.c:原子清理标志及退出、删除、分离路径保护。src/utest/thread_tc.c:重复操作、静态对象复用及双核并发删除回归。src/utest/thread_suspend_tc.c、components/drivers/ipc/utest/workqueue_tc.c:PR 已有的 SMP 测试顺序修正。.github/workflows/utest_auto_run.yml:PR 已有的 QEMU CPU 数量与日志监控修正。bsp/hk32/hk32f030c8-mini/rtconfig.py:GCC debug 从-O0改为-Og,保留调试信息以适配实际 64 KB Flash。本轮最小 CI 修复 / Minimal CI follow-up
相对本轮检查开始时的
8406af15df,净改动仅为 HK32 debug 优化选项及rtdef.h的 clang-format 空白对齐,共 2 个文件、3 行新增和 2 行删除。没有扩大 Flash 容量或关闭功能。src/utest/timer_tc.c已完整恢复,最终 PR 对该文件没有差异。定时器测试的 SMP 同步问题不纳入本次最小修复。Relative to
8406af15df, this follow-up changes only the HK32 debug optimization and cleanup-field formatting. The timer tests are unchanged in the final PR diff.回归覆盖 / Regression coverage
-RT_ERROR且只清理一次。RT_EOK、一次-RT_ERROR和一次 cleanup。请提供验证的 BSP 和 config (provide the config and BSP)
本地验证日期:2026-11-14。工具:Arm GNU GCC 10.3.1、SCons 4.11.1、QEMU ARM 8.0.94、clang-format 22.1.8。配置头均在临时 BSP 副本中通过
.config生成,未提交构建产物。A9 构建使用
python -m SCons -j8 --strict,运行使用 QEMU-M vexpress-a9 -smp 2。HK32 使用普通构建,仍有既有 GPIO 指针类型警告,不将其报告为严格构建通过;其 CI 工具链为 GCC 13.2.1,与本地不同。验证范围 / Validation limits
修复前 HK32 CI 的 soft-I²C 镜像为 65632 B,超限 96 B,编译及格式修复有直接失败证据。A9-SMP CI 曾在原有 core.timer 的精确 tick 断言后超时,但本地原版单次运行通过,未复现该次挂死堆栈。本次不宣称修复该偶发运行问题,也不以改写测试用例来消除它。
The original timer tests passed once locally, but CI previously hit an assertion followed by a timeout. That intermittent runtime issue is not claimed as fixed by this minimal follow-up. Local results do not establish coverage of all architectures or the running-thread exit-versus-delete race.
GitHub Actions
提交
49cfd2f1cf14d37e619222114e6cbadb205a101f的检查已于 2026-11-14 完成:7 个主工作流成功,1 个失败,尚未全部通过。PR Checks 共 109 项成功、4 项按条件跳过、1 项失败,无待执行项。未通过项 / Outstanding failure
A9-SMP kernel/ipc.cfg 编译通过,但运行在
core.mutex / test_dynamic_pri_reverse后停止输出,最终触发 20 分钟超时;没有用例断言失败行。该测试的三个线程非原子递增
volatile int _sync_flag,主线程等待其等于 3,存在 SMP 丢更新后持续等待的风险。CI 没有现场栈或计数值快照,因此这只是与停点相符的风险分析,不能断言已证实此次超时根因。mutex_tc.c在本 PR 中保持不变,未为此扩大测试修改,也未重跑该失败作业。Seven main workflows passed and one failed. All 33 BSP build jobs passed; 46 of 47 utest jobs passed. The unchanged A9-SMP IPC mutex test timed out. A non-atomic completion counter is a plausible source-level risk, but the exact hang state was not captured. The minimal compilation/formatting changes remain the only additions in this follow-up.
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality:
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up